Can this change break existing working code?

Can this change break existing working code?

Yes, it will break anyone depending on options.PropertyNamingPolicy not applying to KeyValuePair properties. We'd have to document this change, which fixes #1197.

It is not good enough to just document these breaking changes. The impact of each breaking change needs to be carefully evaluated. We do not want to end up in situation where people cannot upgrade to .NET 5 because of a ton of unmitigated breaking changes in System.Text.Json.

I believe that you will end up needing to come up with a plan how to make these behavior changes to be opt-in, so the code that works fine against .NET Core 3.x is going to keep working fine against .NET 5.

@jkotas If a switch is needed, consider making this change the default and adding a documented opt-out switch. The behavior introduced by this PR brings things in line with user expectations, whereas before this was just a solitary rough edge that I can attest bites you in real-world scenarios. There are essentially no use cases for wanting the old behavior, besides of course maintaining compatibility when you already wrote code that expects that (buggy) behavior.

@steveharter proposed a new KeyValuePairNamingPolicy option, which if added, would be applied to this change to make it opt-in and non-breaking.

Given that KeyValuePair<,> is just a struct, the PropertyNamingPolicy really should be applied to its property names. If we go in this direction, I think there'd be great value in providing an easy way to opt-out of the breaking behavior, maybe a global boolean option.

In general for 5.0, the goal would be to make behavior changes opt-in, perhaps by providing an opt-in for each behavior change, and/or shared opt-ins for each cluster of related changes. In some cases, it may be difficult to justify why the change is not the default (besides being a breaking change), and we may need to take the break (and provide an easy way to opt out).

Marking this PR "no merge" as the change needs further investigation and consensus.

justify why the change is not the default (besides being a breaking change)

We need to have data to be able to reason about this: Create database of System.Text.Json serialization uses and then evaluate the breaking changes like these against it. Our technical insights team should be able to help you with this.

I think there'd be great value in providing an easy way to opt-out of the breaking behavior, maybe a global boolean option.

A global switches like this tend to not work well as a solution. They affect all uses in your project, and so applying the switch will fix one use but can break another one. This is further complicated by the fact that number of these uses may be internal in packages that you depend on.

Our technical insights team should be able to help you with this.

👍

A global switches like this tend to not work well as a solution.

Not sure if this changes your position, but by "global boolean option", I meant an option on the JsonSerializerOptions class (which won't affect affect the entire project).

Not sure if this changes your position, but by "global boolean option", I meant an option on the JsonSerializerOptions class (which won't affect affect the entire project).

Yes, that would be fine. I would not call this global option. And I do not think it can be an opt-out. It has to be opt-in to be non-breaking so that the existing code that you do not own and are able to change continues to work.

@jkotas This is a pretty bad bug to enshrine forever as default behavior, and code that depends on the buggy behavior will be vanishingly rare. The impact of an opt-out switch would just be that KVPs start round-tripping correctly when using camel case instead of throwing an exception on deserialization.

This is a pretty bad bug to enshrine forever as default behavior,

I won't disagree with this statement.

code that depends on the buggy behavior will be vanishingly rare.

This is fine hypothesis. I would like to see a plan for how we are going to collect data to confirm it for this change, and for number of other behavior changing changes that are proposed for System.Text.Json serializers.

@layomia, @ahsonkhan, where are we with this? If we're not ready to take it because we need to investigate breaking change potential, I'd prefer we close it and only open it again when it's actually ready to be "pulled". Thanks.

@stephentoub, we are still investigating breaking change potential. I'll close it and reopen when it's ready.

Removing the breaking-change label since this was PR closed. #36424 made the changes proposed by this PR.